Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soft enforcement of unit quaternions for node.rotation #971

Merged
merged 1 commit into from
May 31, 2017

Conversation

lexaknyazev
Copy link
Member

Sadly, we can't check quaternion's length with schema, but it's still better than nothing.

@lexaknyazev
Copy link
Member Author

Looks like we should also restrict values of node.scale so that it can't contain zeros.

@pjcozzi
Copy link
Member

pjcozzi commented May 22, 2017

Looks like we should also restrict values of node.scale so that it can't contain zeros.

Although it isn't typical, that is still a valid scale and will create a valid transform matrix, right?

@lexaknyazev
Copy link
Member Author

will create a valid transform matrix, right?

That depends on how we define "valid transform matrix". Scale of 0.0 will produce transform matrix with zero determinant.

@bghgary
Copy link
Contributor

bghgary commented May 22, 2017

Scale of 0.0 will produce transform matrix with zero determinant.

Is this going to cause an issue? I can easily imagine an animation changing the scale to zero for a number of scenarios. It might not have the correct result if this is not allowed. Even if it did have the correct result, it would be a pain to have to tweak the animation to scale to something close to zero.

@pjcozzi
Copy link
Member

pjcozzi commented May 22, 2017

We could add a non-normative/implementation note that states transform matrices may have a zero scale and therefore may will not be invertible in these cases.

@lexaknyazev
Copy link
Member Author

Is this going to cause an issue?

Strictly speaking, such matrix will not be decomposable to TRS (btw, we require this from node.matrix). This will also apply to all children's transformation matrices of a node with zero scale.

... and therefore may will not be invertible in these cases.

So engines won't be able to get normal matrix, right?

@bghgary
Copy link
Contributor

bghgary commented May 22, 2017

Strictly speaking, such matrix will not be decomposable to TRS (btw, we require this from node.matrix). This will also apply to all children's transformation matrices of a node with zero scale.

True, but is this a problem? Animations can only target TRS anyways. The matrix form will be unable to specify a scale of zero.

@pjcozzi
Copy link
Member

pjcozzi commented May 22, 2017

... and therefore may will not be invertible in these cases.
So engines won't be able to get normal matrix, right?

In that case, right.

@lexaknyazev
Copy link
Member Author

In that case, we should properly describe expected behavior.

Let's say that before applying scale, model looks like this (just a tetrahedron):
image

After scaling Y positions to zero, proper normals still exist but getting them without normal matrix could be difficult and likely not all engines will bother.
image

@lexaknyazev
Copy link
Member Author

Since scale can be negative, we can't definitely say what orientations should normals have in case of zero scale. That's not a big deal for animations, because engines can determine orientation from adjacent keyframes.

But static transforms are ambiguous.

@emackey
Copy link
Member

emackey commented May 23, 2017

Is there any reasonable way to allow scaling to [0, 0, 0] but forbid scaling individual axes to 0?

@bghgary
Copy link
Contributor

bghgary commented May 24, 2017

This is what Unity does when only one of the scale component is zero.

Original scale (1, 1, 1)
image

X scale is zero (0, 1, 1)
image

@lexaknyazev
Copy link
Member Author

Should this be expected behavior for other renderers?

Other options include:

  • rendering is undefined;
  • mesh is not rendered;
  • engine uses very small value instead of 0.0, tries to deduce its sign.

@emackey
Copy link
Member

emackey commented May 24, 2017

  • rendering is undefined

Sounds OK. Remember it's not just normal vectors that are undefined: All of the polygons become co-planar, so depth buffering may be undefined as well.

@pjcozzi
Copy link
Member

pjcozzi commented May 25, 2017

Thanks for looking into Unity, @bghgary! I suspect this was not designed intentionally in Unity.

rendering is undefined;

Could be OK, but we try to minimize undefined behavior, so why not go with

mesh is not rendered;

unless we have a good use case for this? Paper Mario? 😄

@bghgary
Copy link
Contributor

bghgary commented May 26, 2017

BabylonJS does this if I force the x-scale to zero:

image

Note the z-fighting with the triangles.

I think whatever we do here, it will be an implementation note. I don't think we should restrict this in the spec. I would probably leave it as undefined. Practically, I doubt anyone will do this.

@snagy and I just noticed another thing we probably should specify in the spec. When an odd number of scale components are negative, the faces of the triangles should be flipped. This works correctly in Unity but not currently in BabylonJS. Making the scale negative is useful for mirroring.

@lexaknyazev
Copy link
Member Author

When an odd number of scale components are negative, the faces of the triangles should be flipped.

We assume here, that mirroring is more usable, than flipping object inside-out, right?
Also, I'm wondering how should it work for nested transforms, which could be defined by both matrix and scale (in different nodes, of course).

@bghgary
Copy link
Contributor

bghgary commented May 26, 2017

We assume here, that mirroring is more usable, than flipping object inside-out, right?

Yes. I suppose there might be a use case for double-sided materials, but it will be uncommon.

nested transforms

My understanding is that the determinant of the matrix can be used to decide whether to flip the faces or not. Chapter 3.1.2 of Mathematics for 3D Game Programming and Computer Graphics by Eric Lengyel describes this. The gist of it is that if the determinant is negative, then the handedness is reversed, which means the faces should be flipped. @snagy checked Unreal's implementation and that's what it does.

@lexaknyazev
Copy link
Member Author

Relying on determinant sounds good (I was a bit confused by counting the number of negative scale components).

Could you please update this PR with a note on these topics (zero/negative determinant)?

@bghgary
Copy link
Contributor

bghgary commented May 30, 2017

I created a separate PR. Hope that's okay.

@lexaknyazev lexaknyazev merged commit 75be81f into 2.0 May 31, 2017
@lexaknyazev lexaknyazev deleted the node-rotation-fix branch May 31, 2017 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants